Resolve index names in indices_boost #21393

masaruh · 2016-11-08T02:25:11Z

This change allows specifying alias/wildcard expression in indices_boost.
And added another format for specifying indices_boost. It accepts array of index name and boost pair.
If an index is included in multiple aliases/wildcard expressions, the first match will be used.

Closes #4756

clintongormley · 2016-11-24T17:53:08Z

ping @javanna

javanna

I left some comments, looks good though, sorry it took me ages to get to this ;)

javanna · 2016-11-29T09:35:10Z

core/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java

+        }
+
+        List<Tuple<String, Float>> concreteIndexBoost = new ArrayList<>();
+        for (Tuple<String, Float> ib : indexBoost) {


can you save ib.v1() and ib.v2() to a variable so it is more readable what they hold?

Right.
I didn't like tuple since it's not very readable as you pointed out. So, I decided to create a new class.

javanna · 2016-11-29T09:36:20Z

core/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java

+        List<Tuple<String, Float>> concreteIndexBoost = new ArrayList<>();
+        for (Tuple<String, Float> ib : indexBoost) {
+            String[] concreteIndices =
+                indexNameExpressionResolver.concreteIndexNames(clusterState, IndicesOptions.lenientExpandOpen(), ib.v1());


why hardcoding lenient indices options? maybe use strict? or even better, why not applying the options from the search request so we are consistent with how indices are resolved in the url?

Done.
But I'm wondering if it makes this change breaking. Specifying index that doesn't exist used to be no op. Now returns error.

good point, I think it's ok if it is configurable. and then it should do by default the same as the rest of the same search request does.

javanna · 2016-11-29T09:56:33Z

core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

        return this;
    }

    /**
     * Gets the boost a specific indices will receive when the query is
     * executed against them.
     */
-    public ObjectFloatHashMap<String> indexBoost() {
+    public List<Tuple<String, Float>> indexBoost() {


very small change, it's a getter that is probably not used (I assume most people just set the index boost), but it is a breaking change for the java api. Not a huge deal though, shall we add the breaking-java label to this PR?

javanna · 2016-11-29T10:00:24Z

core/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java

+            }
+        }
+
+        searchRequest.source().indexBoost(concreteIndexBoost);


I find it odd that we modify the original source builder with the resolved indices names and replace what we originally had. Would it be possible to transport the resolved indices differently? I think we should serialize this new info separately and carefully handle bw compatibility around that.

javanna · 2016-11-29T10:01:14Z

core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

+     * @param indexBoost
+     *            The boosts to apply to the indices
+     */
+    public SearchSourceBuilder indexBoost(List<Tuple<String, Float>> indexBoost) {


see comment above, ideally this setter wouldn't be needed, as it's here only because we need it internally to replace the provided indices with their resolved names.

This was removed.

javanna · 2016-11-29T10:10:21Z

core/src/test/java/org/elasticsearch/search/indicesboost/SimpleIndicesBoostSearchIT.java

+        logger.info("Query with alias_test1 boosted");
+        SearchResponse response = client().search(searchRequest()
+            .searchType(SearchType.QUERY_THEN_FETCH)
+            .source(searchSource().explain(true).indexBoost("alias1", indexBoost).query(termQuery("test", "value")))


do we need explain?

This test was removed in favor of yaml tests.

javanna · 2016-11-29T10:10:59Z

core/src/test/java/org/elasticsearch/search/indicesboost/SimpleIndicesBoostSearchIT.java


-        logger.info("--- DFS_QUERY_THEN_FETCH");
+    public void testIndicesBoostWithAlias() throws Exception {
+        createIndex("alias_test1", "alias_test2");


maybe call the concrete indices "index1" and "index2", otherwise one may think they are aliases :)

This test was removed in favor of yaml tests.

javanna · 2016-11-29T10:11:15Z

core/src/test/java/org/elasticsearch/search/indicesboost/SimpleIndicesBoostSearchIT.java

+
+        float indexBoost = 1.1f;
+
+        logger.info("--- QUERY_THEN_FETCH");


I think we can remove this

This test was removed in favor of yaml tests.

javanna · 2016-11-29T10:12:27Z

core/src/test/java/org/elasticsearch/search/indicesboost/SimpleIndicesBoostSearchIT.java

+            .searchType(SearchType.QUERY_THEN_FETCH)
+            .source(searchSource()
+                .explain(true)
+                .indexBoost("non_existent", boost)


I see why you used lenient indices options, so that nothing breaks if non existing indices are specified. But maybe that should be configurable like I commented above.

javanna · 2016-11-29T10:14:12Z

core/src/test/java/org/elasticsearch/search/indicesboost/SimpleIndicesBoostSearchIT.java

+
+        assertThat(response.getHits().totalHits(), equalTo(1L));
+        logger.info("Hit[0] {} Explanation {}", response.getHits().getAt(0).index(), response.getHits().getAt(0).explanation());
+        assertThat(response.getHits().getAt(0).index(), equalTo("test1"));


given that you added yaml tests, I wonder if these integ tests are still needed. Maybe we can convert them all to yaml tests?

masaruh · 2016-12-01T13:10:13Z

core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

            }
-            builder.endObject();
+            builder.endArray();
        }



Wonders if I should put add concreteIndexBoosts here. Since it's used only internally, it's not needed?

no, I think we should print out what the user sent, not our own resolved objects.

I see. Thanks for the clarification.

masaruh · 2016-12-01T13:17:55Z

Thanks @javanna ! Addressed your feedback.

javanna

thanks @masaruh for addressing the comments, I left some more, but it's really close.

javanna · 2016-12-01T14:29:27Z

core/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java

@@ -107,6 +107,10 @@ protected void doExecute(Task task, SearchRequest searchRequest, ActionListener<
            searchRequest.preference());
        failIfOverShardCountLimit(clusterService, shardIterators.size());

+        if (searchRequest.source() != null) {
+            searchRequest.source().resolveIndexBoosts(indexNameExpressionResolver, clusterState, searchRequest.indicesOptions());


can we move the resolution method out of SearchSourceBuilder too and also transport the info out of it as well? Like we do with aliasFilters map for instance? Does it make sense? Trying to keep SearchSourceBuilder as POJO as possible and have logic outside of it.

Makes sense. Done.

javanna · 2016-12-01T14:30:37Z

core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

     * @param indexBoost
     *            The boost to apply to the index
     */
    public SearchSourceBuilder indexBoost(String index, float indexBoost) {
-        if (this.indexBoost == null) {
-            this.indexBoost = new ObjectFloatHashMap<>();
+        if (index == null) {


Objects.requireNonNull?

javanna · 2016-12-01T14:31:42Z

core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

+            boost = in.readFloat();
+        }
+
+        public IndexBoost(QueryParseContext context) throws IOException {


does it need to be public?

same for the other constructors too

javanna · 2016-12-01T14:32:40Z

core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

+            }
+        }
+
+        public String index() {


use the get prefix?

javanna · 2016-12-01T14:32:45Z

core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

+            return index;
+        }
+
+        public float boost() {


get prefix?

javanna

I really like this @masaruh thanks for addressing my comments, I left some new ones but mainly minors. I promise it's the last changes I'm asking you to make ;)

javanna · 2016-12-06T20:42:52Z

core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

-        out.writeVInt(indexBoostSize);
-        if (indexBoostSize > 0) {
-            writeIndexBoost(out);
+        if (indexBoosts == null) {


Should we initialize indexBoosts to new ArrayList<>() straight-away instead and remove this null invariant? This if would go away too.

javanna · 2016-12-06T20:43:29Z

core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

-                indexBoost.put(in.readString(), in.readFloat());
-            }
+        List<IndexBoost> boosts = in.readList(IndexBoost::new);
+        if (boosts.size() > 0) {


I think that we can unconditionally assign indexBoosts.

True.
I was trying to preserve state where indexBoosts isn't set (null). But if we initialize indexBoosts with empty array, no need to worry.

javanna · 2016-12-06T20:44:08Z

core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

+        Objects.requireNonNull(index);
+
+        if (this.indexBoosts == null) {
+            this.indexBoosts = new ArrayList<>();


if the list is already initialized like I suggested above, this if goes away too

javanna · 2016-12-06T20:45:19Z

core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

                        }
                    }
+                } else if (context.getParseFieldMatcher().match(currentFieldName, INDICES_BOOST_FIELD)) {
+                    indexBoosts = new ArrayList<>();


rather than re-initializing the list, shall we enforce the fact that users cannot specify boost in both ways? Either deprecated or new syntax, not both at the same time?

javanna · 2016-12-06T20:48:24Z

core/src/main/java/org/elasticsearch/search/internal/ShardSearchLocalRequest.java

@@ -167,6 +175,9 @@ protected void innerReadFrom(StreamInput in) throws IOException {
        source = in.readOptionalWriteable(SearchSourceBuilder::new);
        types = in.readStringArray();
        aliasFilter = new AliasFilter(in);
+        if (in.getVersion().onOrAfter(Version.V_5_2_0_UNRELEASED)) {
+            this.indexBoost = in.readFloat();


maybe we should be more explicit and initialize indexBoost in an else branch here, rather than above when declaring it. I think de-serialization is the only scenario when it may not get assigned.

javanna · 2016-12-06T20:50:25Z

docs/build.gradle

@@ -369,3 +369,18 @@ buildRestTests.setups['range_index'] = '''
      body: |
        {"index":{"_id": 1}}
        {"expected_attendees": {"gte": 10, "lte": 20}, "time_frame": {"gte": "2015-10-31 12:00:00", "lte": "2015-11-01"}}'''
+
+// Used by index boost doc
+buildRestTests.setups['index_boost'] = '''


cool stuff I had no idea you could do this ;)

javanna · 2016-12-06T20:52:07Z

rest-api-spec/src/main/resources/rest-api-spec/test/search/40_indices_boost.yaml

+        index: [test_1, test_2]
+
+---
+"Indices boost using object":


we need to add a skip section to every test relying on warnings, or the runners not supporting the "warnings" feature will break when trying to run this test.

javanna · 2016-12-06T21:00:35Z

core/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java

+        Map<String, Float> concreteIndexBoosts = new HashMap<>();
+        for (SearchSourceBuilder.IndexBoost ib : source.indexBoosts()) {
+            String[] concreteIndices =
+                indexNameExpressionResolver.concreteIndexNames(clusterState, searchRequest.indicesOptions(), ib.getIndex());


Can we switch to using the method that returns Index[] and use the index uuid rather than the index name as key in the map? That will help with the upcoming cross cluster search feature ;)

javanna · 2016-12-06T21:02:04Z

@clintongormley maybe you want to have a look at the updated docs?

masaruh · 2016-12-07T14:24:44Z

Thank you again @javanna. Hopefully changes are sufficient :)

clintongormley · 2016-12-07T14:31:21Z

Docs LGTM

javanna

Heya @masaruh I had one last look. Left a few minors, LGTM otherwise. No need for another review round. Thanks for your patience ;)

javanna · 2016-12-07T20:34:50Z

core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

-            this.indexBoost = new ObjectFloatHashMap<>();
-        }
-        this.indexBoost.put(index, indexBoost);
+        Objects.requireNonNull(index);


add a message as second argument?

javanna · 2016-12-07T20:37:22Z

core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

-                if (keys[i] != null) {
-                    builder.field((String) keys[i], values[i]);
-                }
+        if (indexBoosts != null) {


can this ever be null?

I think that it can rather be empty, hence we always print out the array, which sounds ok to me. I think the null check can go away.

True. Removed.

javanna · 2016-12-07T20:46:05Z

core/src/main/java/org/elasticsearch/search/internal/ShardSearchLocalRequest.java

@@ -85,17 +87,19 @@ public ShardSearchLocalRequest(ShardId shardId, String[] types, long nowInMillis
        this.nowInMillis = nowInMillis;
        this.aliasFilter = aliasFilter;
        this.shardId = shardId;
+        indexBoost = 1.0f;


this doesn't matter as explain and validate query apis don't support the indices_boost section right?

For example, if we don't set it here, boost is 0. Then, validate query with explain=true returns parsed queries with boost 0. (Actually, several tests fails if it's not set here)
So, I leave it as is for now. If it's not right, let me know. I'll fix.

I see why it's 1 and not 0, and I think it's correcy. my question was rather why we don't pass it in like in the other constructor, I think it is because the apis that use this constructor don't support indices_boost right? just trying to understand

Yes, I believe so.

javanna · 2016-12-07T20:48:27Z

core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

@@ -1002,15 +977,19 @@ public void parseXContent(QueryParseContext context, AggregatorParsers aggParser
                        scriptFields.add(new ScriptField(context));
                    }
                } else if (context.getParseFieldMatcher().match(currentFieldName, INDICES_BOOST_FIELD)) {
-                    indexBoost = new ObjectFloatHashMap<>();
+                    if (!indexBoosts.isEmpty()) {


I am sorry, I didn't think this through. The two sections have the same name, so the json wouldn't even be valid if they were both there. I think we should not check it explicitly, otherwise we would have to do it for every single field in our parsers, which we don't do. Relates to #19614

Oh, sorry I misunderstood you. Removed duplicate check.

no there was no misunderstanding, I just didn't realize the implications of this check

javanna · 2016-12-07T21:14:05Z

core/src/main/java/org/elasticsearch/search/DefaultSearchContext.java

@@ -173,6 +173,7 @@
        this.timeout = timeout;
        queryShardContext = indexService.newQueryShardContext(request.shardId().id(), searcher.getIndexReader(), request::nowInMillis);
        queryShardContext.setTypes(request.types());
+        queryBoost = request.indexBoost();


I think that we can remove the query boost setter and make the instance member final.

javanna

@masaruh I left one single comment, quite important though, on bw comp. Sorry I should have noticed this before.

javanna · 2016-12-10T09:45:29Z

core/src/main/java/org/elasticsearch/search/internal/ShardSearchLocalRequest.java

+        if (in.getVersion().onOrAfter(Version.V_5_2_0_UNRELEASED)) {
+            indexBoost = in.readFloat();
+        } else {
+            indexBoost = 1.0f;


I think that we have a problem with backwards compatibility here. Most likely our bw comp tests are going to fail (would be nice to verify that actually). I think indices_boost would get ignored in a mixed version cluster (e.g. 5.x and 5.1):

5.x coord node does the resolution on the coord node, serializes search source builder to 5.1 node (nothing changed there) which doesn't know how to resolve indices names yet, hence it reads indices boost from the original source, it works as it used to. That's ok. We can't do better than this.

5.1 coord node doesn't resolve indices boost on the coord node, serializes source builder to 5.x node (nothing changed there) which knows how to resolve indice names, but it doesn't get them through shard search request as 5.1 node didn't send the info. Hence the indices boost are always going to be 1, ignored. We should fix this. I think the way to fix it would be to detect based on the version whether we have to read the index boost from the source builder of from the shard search request.

ooh, indeed. Will fix.

Actually tested and work just like you said.
Fixed.

nice! fix looks good! did you have some tests out of qa/backwards-5.0 fail without the fix? Or did you just test manually? The backwards tests run the rest tests against a mixed version cluster, so I think your newly added tests should fail in that setup. Can you verify? If so we are good to go!

it would also be wise to unit test that newly added bw comp layer. We have something similar in ShardSearchTransportRequest for alias filters (we moved their parsing on the coord node with 5.1).

I tested manually without/with the fix.
I added test similar to alias filters.

javanna · 2016-12-10T20:26:05Z

core/src/test/java/org/elasticsearch/search/internal/ShardSearchTransportRequestTests.java

@@ -96,7 +96,7 @@ private ShardSearchTransportRequest createShardSearchTransportRequest() throws I
            filteringAliases = new AliasFilter(null, Strings.EMPTY_ARRAY);
        }
        return new ShardSearchTransportRequest(searchRequest, shardId,
-                randomIntBetween(1, 100), filteringAliases, Math.abs(randomLong()));
+                randomIntBetween(1, 100), filteringAliases, 1.0f, Math.abs(randomLong()));


can you randomize the boost here so we also test boost serialization from 6.x to 6.x with a value that's different from default? You could do randomBoolean() ? 1.0f : randomFloat() ?

Right. Done.

javanna · 2016-12-10T20:26:07Z

core/src/test/java/org/elasticsearch/search/internal/ShardSearchTransportRequestTests.java

+            BytesStreamOutput output = new BytesStreamOutput();
+            output.setVersion(Version.V_5_0_0);
+            readRequest.writeTo(output);
+            assertEquals(output.bytes().toBytesRef(), requestBytes.toBytesRef());


thanks a lot for adding this!

masaruh · 2016-12-11T08:02:21Z

rest-api-spec/src/main/resources/rest-api-spec/test/search/40_indices_boost.yaml

+  - match: { hits.hits.1._index: test_1 }
+
+---
+"Indices boost using array":


Excluded clients < 5.2.0 for tests that use array format.

This change allows specifying alias/wildcard expression in indices_boost. And added another format for specifying indices_boost. It accepts array of index name and boost pair. If an index is included in multiple aliases/wildcard expressions, the first match will be used. With new format, old format is marked as deprecated. Closes elastic#4756

javanna

very nice @masaruh LGTM

masaruh · 2016-12-16T06:06:26Z

Thanks @javanna for thorough review!

masaruh · 2016-12-16T07:08:45Z

Ugh, I should have rebase before pushing merge button...
Test fail on SearchSourceBuilderTests.testToXContent, which is good.
~~Needs a few adjustment I'll revert the commit for now.~~
Will fix it.

javanna · 2016-12-16T08:59:45Z

no worries @masaruh , and thanks for fixing.

masaruh added >enhancement review :Search/Search Search-related issues that do not fall into other categories v6.0.0-alpha1 v5.1.1 labels Nov 8, 2016

clintongormley added :Query DSL and removed :Search/Search Search-related issues that do not fall into other categories labels Nov 8, 2016

javanna self-assigned this Nov 10, 2016

javanna requested changes Nov 29, 2016

View reviewed changes

javanna added v5.2.0 and removed v5.1.1 labels Nov 29, 2016

masaruh added the >breaking-java label Dec 1, 2016

masaruh force-pushed the alias_boost branch 2 times, most recently from 0f62fa9 to e036efa Compare December 1, 2016 13:01

masaruh commented Dec 1, 2016

View reviewed changes

javanna requested changes Dec 1, 2016

View reviewed changes

masaruh force-pushed the alias_boost branch 2 times, most recently from 4c9a13c to 7e96f67 Compare December 3, 2016 12:17

javanna approved these changes Dec 6, 2016

View reviewed changes

masaruh force-pushed the alias_boost branch from 7e96f67 to 2b07a5d Compare December 7, 2016 14:24

javanna approved these changes Dec 7, 2016

View reviewed changes

masaruh force-pushed the alias_boost branch from 2b07a5d to 36f2e62 Compare December 10, 2016 08:48

javanna requested changes Dec 10, 2016

View reviewed changes

masaruh force-pushed the alias_boost branch 2 times, most recently from 5c28135 to 476a246 Compare December 10, 2016 14:49

javanna reviewed Dec 10, 2016

View reviewed changes

masaruh force-pushed the alias_boost branch 3 times, most recently from 729dce7 to 03428c2 Compare December 11, 2016 07:59

masaruh commented Dec 11, 2016

View reviewed changes

masaruh force-pushed the alias_boost branch from 03428c2 to 00fe4dc Compare December 11, 2016 09:32

masaruh force-pushed the alias_boost branch from 00fe4dc to 3df2a08 Compare December 11, 2016 13:42

javanna approved these changes Dec 13, 2016

View reviewed changes

masaruh merged commit a0185c8 into elastic:master Dec 16, 2016

masaruh mentioned this pull request Dec 16, 2016

Revert "Resolve index names in indices_boost" #22213

Closed

clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query DSL labels Feb 14, 2018


		float indexBoost = 1.1f;

		logger.info("--- QUERY_THEN_FETCH");

Resolve index names in indices_boost #21393

Resolve index names in indices_boost #21393

Conversation

masaruh commented Nov 8, 2016

clintongormley commented Nov 24, 2016

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masaruh Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masaruh commented Dec 1, 2016

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Dec 6, 2016

masaruh commented Dec 7, 2016

clintongormley commented Dec 7, 2016

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masaruh Dec 1, 2016 •

edited

Loading